Improve decoder throughput with buffered scalar reads - #299
Open
Saurabh Singh (saurabh500) wants to merge 3 commits into
Open
Improve decoder throughput with buffered scalar reads#299Saurabh Singh (saurabh500) wants to merge 3 commits into
Saurabh Singh (saurabh500) wants to merge 3 commits into
Conversation
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 15, 2026 00:44
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Optimizes TDS row decoding by synchronously consuming buffered scalar values while retaining the existing asynchronous packet-refill fallback.
Changes:
- Adds optional scalar probes to
TdsPacketReader. - Implements buffered byte,
u16, andi32reads forNetworkTransport. - Routes decoder hot paths through sync-first reads and adds boundary tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mssql-tds/src/io/packet_reader.rs |
Defines non-consuming scalar probe methods. |
mssql-tds/src/datatypes/decoder.rs |
Uses sync-first scalar reads in decoder paths. |
mssql-tds/src/connection/transport/network_transport.rs |
Delegates probes and retains packet-refill fallback. |
mssql-tds/src/connection/transport/buffers.rs |
Implements scalar probes and unit tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-tds/src/connection/transport/network_transport.rsmssql-tds/src/datatypes/decoder.rs🔗 Quick Links |
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 15, 2026 16:23
View session
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bf6f59e-c587-4687-93bd-201a9f26681a
5 tasks
Saurabh Singh (saurabh500)
force-pushed
the
dev/saurabh/sync-buffered-reads
branch
from
August 15, 2026 17:50
3fc0b77 to
0c25b66
Compare
Saurabh Singh (saurabh500)
changed the base branch from
main
to
dev/saurabh/native-resultset-futures
August 15, 2026 17:50
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 15, 2026 17:52
View session
5 tasks
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 15, 2026 18:28
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bf6f59e-c587-4687-93bd-201a9f26681a
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Improves row-decoder throughput by synchronously consuming fixed-width scalars that are already buffered in
NetworkTransport. A buffer miss falls back to the existing async TDS packet read/refill path and retries, so packet framing and error handling remain authoritative.The optimized decoder reads cover byte, little-endian
i16,u16, 24-bitu32,i32,u32, 40-bitu64,i64,f32, andf64. This is a standalone runtime optimization informed by measurements from #269. It does not revive that PR or retry decoder convergence.Stack position
This draft is stacked directly on #291 (
Remove ResultSet async_trait boxing) in native stack #287. Its incremental diff is the two buffered-read commits above #291; the current open chain is #286 → #291 → this PR.Implementation
TdsPacketReaderconfigurations, with safeNonedefaults for other readers.TdsReadBufferand delegates them fromNetworkTransport.read_sync_first!macro at every production fixed-width scalar read indatatypes/decoder.rs.read_tds_packet()as the only refill path for async misses.u64reads outside the decoder unchanged.Before and after
Before: every scalar enters the async path
sequenceDiagram participant D as Decoder participant A as async read_*() participant B as TdsReadBuffer participant P as Packet I/O D->>A: read_*().await Note over D,A: Construct and poll a future for every scalar A->>B: Enough bytes buffered? alt Buffered hit B-->>A: Yes A->>B: Consume N bytes A-->>D: Ready(value) else Buffer miss B-->>A: No A->>P: read_tds_packet().await P-->>A: Next framed payload A->>B: Retry and consume N bytes A-->>D: Ready(value) endAfter: synchronous probe before async fallback
sequenceDiagram participant D as Decoder participant M as sync-first macro participant T as NetworkTransport participant B as TdsReadBuffer participant P as Packet I/O D->>M: read_sync_first! M->>T: try_read_*() [sync] T->>B: Probe complete N-byte scalar alt Buffered hit B-->>T: Some(value), consume N bytes T-->>M: Some(value) M-->>D: value Note over D,M: No async future constructed or polled else Buffer miss B-->>T: None, consume zero bytes T-->>M: None M->>T: read_*().await T->>P: read_tds_packet().await P-->>T: Next framed payload T->>B: Refill, retry, consume complete scalar T-->>M: Ok(value) M-->>D: value endNis the scalar's fixed wire width (1, 2, 3, 4, 5, or 8 bytes). Packet framing, cancellation, encryption, and errors remain in the existing async fallback.Correctness coverage
Targeted tests cover successful buffered reads, zero consumption when any supported scalar is incomplete, and every supported scalar split across real TDS packet boundaries through
NetworkTransport. Existing NULL and length-marker handling remains unchanged.Production-reader benchmarks
All benchmarks use concrete
NetworkTransport, packetized in-memory input, 7,992-byte packet payloads, 30,000 rows per pass, 2 warmups, 9 measured passes per cell, and 8 paired rounds alternating baseline/candidate order. Negative deltas are faster.Stacked result versus #291 (
8c6ff8b2)The stack comparison used identical lockfiles and isolated source/target directories. The #291 and stacked test binaries had different SHA-256 hashes.
Initial change versus clean
main(ac1023a1)Expanded fixed-width reads versus the first draft (
47c2fb68)The
fixed_scalarsworkload has 64 columns:Int1,Int2,Int4,Int8,Flt4,Flt8,DateTime, andDateTim4, repeated eight times.The optimized pre-stack benchmark executable grew by about 192 KB (1.75%), so code-size impact remains a review consideration.
Validation
cargo bfmtcargo bclippy$env:RUSTFLAGS='--cfg fuzzing'; cargo check -p mssql-tds --libmssql-tdslibrary nextest suite with generated TLS fixtures: 1,764 passedmain...HEADcoverage: 98% diff coverage and 91.5% overall coveragecargo btestwas attempted locally before stacking but the live SQL integration tests fail with SchannelSEC_E_WRONG_PRINCIPAL.Risk
The fast path is limited to already-buffered decoder scalars on
NetworkTransport. Other packet readers retain their existing async behavior through defaultNoneprobes. Misses preserve partial bytes and reuse the existing refill path, including cancellation and encryption behavior. The broader set of inlined probes trades some code size for lower per-value async overhead.Related Issues
Related to #247.
Stacked on #291.
Historical measurement context: #269.
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses locally (environment-blocked; pre-stack remote matrix passes)